-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
adds a signature::Signer
interface
#537
base: main
Are you sure you want to change the base?
adds a signature::Signer
interface
#537
Conversation
85783cc
to
6525f15
Compare
336466d
to
8145f8e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for reviewing a WIP though I found it a little bit interesting so I couldn't help my self. Feel free to disregard anything I have commented on.
tss-esapi/tests/integration_tests/abstraction_tests/public_tests.rs
Outdated
Show resolved
Hide resolved
df740fd
to
eaf82d5
Compare
Oh, no, thanks for reviewing it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for bringing the patch! The overall design looks good to me.
// Note: this does not implement `TryFrom<RsaSignature>` because `RsaSignature` does not carry the | ||
// information whether the signatures was generated using PKCS#1v1.5 or PSS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fields of RsaSignature
are private, so we can extend it to capture this detail as well. It's a deviation from the TPM spec, but I don't necessarily see a problem with it. Thoughts?
cc @Superhepper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it is not really a problem as long as it does not causes any ambiguities in the conversions between TPMS_SIGNATURE_RSA
and RsaSignature
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine like that, the comment was more for a future self why we would not have such a TryFrom
use signature::{DigestSigner, Error as SigError, KeypairRef}; | ||
|
||
#[derive(Debug)] | ||
pub struct Ecdsa<'ctx, C> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I know this is a draft: ) Would be good to have some docs on these structs to make it clear what they're meant for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to add some, and add a code sample in the doc as well.
/// Key parameters for this curve | ||
pub fn key_params<D>() -> KeyParams |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit mystified by the purpose of this function. Also by the use of "this curve" in the doc for it, given that Ecdsa
is presumably not the description of a curve (?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C
describes the curve (could be NistP256
, NistP384
, NistP521
, ...) those are all supported here. When using the signer (through the Ecdsa
struct) you would specify which curve you're using.
This would pick the correct parameters to specify to the TPM when signing, the size of the object that comes back from signature, how to verify them, ...
This function just creates the TPM parameters related to this curve and the selected digest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I think I was mostly confused by the existence of two nearly-identical methods before, and that the struct they're tacked to doesn't represent (just) a specific curve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key_params_default
just use the "default" hashing mechanism for a given curve (P256 would use sha256, P384 would use sha384, etc).
The hashing methods needs to line up otherwise the curve has a higher "security score" than the hashing mechanism it uses and it's a waste of space.
fe52b1c
to
fa8ff3a
Compare
7bcbb0c
to
b79e193
Compare
tss-esapi/tests/integration_tests/abstraction_tests/transient_key_context_tests.rs
Show resolved
Hide resolved
tss-esapi/tests/integration_tests/abstraction_tests/transient_key_context_tests.rs
Show resolved
Hide resolved
b79e193
to
9f760ad
Compare
9f760ad
to
edf67ed
Compare
edf67ed
to
73e0b9c
Compare
ad664c0
to
a699a3e
Compare
@baloo why EcSigner depends on TransientKeyContext? The only method is used there is sign(). Besides
I should probably explain the use case: I'm loading a key that is stored on TPM as Persistent and currently there is no way to use EcSigner with such keys. TransientTpmContext is transient for reasons but then it limits usage of EcSigner Sure, I can implement my own signer but if we can make EcSigner more versatile it would be very cool |
This is a good point. I think I like the idea of making a trait but I'm not sure how to do it yet, |
I have a rough follow-up PR for the trait option: baloo#1 |
It was also missing the implementation for an RSA signer: baloo#2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good overall!
Are you intending to add signing support for RSA too?
Ah, one thing that I keep forgetting - could you please document the new feature (in the README)? |
Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>
a37e0ec
to
e8c25df
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good. While I'm no expert in rust crypto I'm of the opinion that this is ready to be shipped to folks to try out in the wild.
Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>
Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>
Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>
Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>
Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>
Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>
Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>
Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>
Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>
e8c25df
to
2cc63f4
Compare
anything else needed here? |
@baloo if possible I'd like to do one last test with x509 and TLS coming weekend. Otherwise looks great! |
@rucoder just so you know, there are a bunch of improvements in the upcoming release of The trait-based approach proved much more flexible for us. #563 should pull the pre-releases if you wanted to try that. |
This brings an implementation of a
signature::Signer
for keys stored on the TPM.This is intend to make for easier re-use of this crate and to allow to:
Here is an implementation of an SSH agent making use of that infrastructure: wiktor-k/ssh-agent-lib#87